-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(AnalyticalTable): add experimental feature to determine column widths based on content #295
Conversation
@@ -56,39 +61,39 @@ const columns = [ | |||
const data = generateData(200); | |||
const dataTree = generateData(20, true); | |||
|
|||
export const defaultTable = () => { | |||
const renderTable = () => { | |||
// const innerData = generateData(200); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove
// const innerData = generateData(200); |
@@ -56,39 +61,38 @@ const columns = [ | |||
const data = generateData(200); | |||
const dataTree = generateData(20, true); | |||
|
|||
export const defaultTable = () => { | |||
const renderTable = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -259,6 +259,7 @@ const makeTreeEntry = () => ({ | |||
|
|||
const makeEntry = () => ({ | |||
name: getRandomName(), | |||
longColumn: 'Really really long column content... don´t crop please', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add the long column to the treeTableData as well? otherwise this column is always empty. Or change the columns for the treeTable back to the old config.
} = props; | ||
|
||
const classes = useStyles({ rowHeight: props.rowHeight }); | ||
const [internalRows, setInternalRows] = useState([]); | ||
const [hiddenColumns, setHiddenColumns] = useState([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this shadow on the hidden columns? can't we just call useDynamicColumnWidths
after useTable
and use the columns from the tableState?
} = props; | ||
|
||
const classes = useStyles({ rowHeight: props.rowHeight }); | ||
const [internalRows, setInternalRows] = useState([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need that? why not using the rows from the table direclty?
const updateTableClientWidth = useCallback(() => { | ||
requestAnimationFrame(() => { | ||
if (tableRef.current) { | ||
setTableClientHeight(tableRef.current.clientWidth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clientHeight === clientWidth?
charLength < 15 ? Math.sqrt(charLength * 1500) : 8 * charLength; | ||
const approximateContentPxFromCharLength = (charLength) => 8 * charLength; | ||
|
||
export const useDynamicColumnWidths = (scaleWidthMode, columns, rows, totalWidth, hiddenColumns, setColumnWidth) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we run this as a tableHook? just the our useXYZStyle hooks? then we would have access to all table data like columns, row, width, hidden columns, etc...
# Conflicts: # packages/main/src/components/AnalyticalTable/index.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two code snippets that can be removed in my opinion.
Could you please run prettier on these files before merging? looks like the pre-commit hook did not run.
Other than that looks good 🚢
@@ -59,6 +60,7 @@ const dataTree = generateData(20, true); | |||
export const defaultTable = () => { | |||
return ( | |||
<div style={{ display: 'flex', flexDirection: 'column' }}> | |||
{' '} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can remove that line?
} = props; | ||
|
||
const classes = useStyles({ rowHeight: props.rowHeight }); | ||
// const [tableClientWidth, setTableClientWidth] = useState(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be removed?
…AP/ui5-webcomponents-react into feat/experimental-dynamic-table-widths
No description provided.